Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add SchemeSupported to the OpenURI portal #1203

Merged
merged 1 commit into from
Oct 11, 2024

Conversation

xhorak
Copy link
Contributor

@xhorak xhorak commented Nov 16, 2023

To check whenever specific scheme is supported by the host.

@grulja
Copy link
Contributor

grulja commented Jan 16, 2024

I think this change as it is, is a bit incomplete. If an app/client is supposed to call this API, it needs to be added to the org.freedesktop.portal.OpenURI instead, because your addition is what backend implementations (xdp-kde/xdp-gnome) are supposed to implement, but these APIs are not called directly by clients and possibly not even visible outside the sandbox by default. With an additional API, you should also bump a version of the org.freedesktop.portal.OpenURI so clients can check whether the version of xdp that is running on the host supports it. I'm not sure a backend counterpart is needed as you can probably tell the host can handle given scheme using the g_app_* APIs (I'm not an expert in this regard). That would mean that also the implementation of this additional API is missing in this MR.

tests/test-portals.c Outdated Show resolved Hide resolved
src/open-uri.c Outdated Show resolved Hide resolved
@grulja
Copy link
Contributor

grulja commented Jan 23, 2024

@xhorak also please squash your commits or split it to reasonable parts and remove the merge commit.

@xhorak xhorak closed this Jan 23, 2024
@xhorak xhorak reopened this Jan 23, 2024
@xhorak xhorak changed the title Add SchemeSupported to the AppChooser Add SchemeSupported to the OpenURI portal Jan 31, 2024
@xhorak
Copy link
Contributor Author

xhorak commented Jan 31, 2024

@matthiasclasen Could you please check this one?

@grulja
Copy link
Contributor

grulja commented Jun 10, 2024

This needs a rebase.

Also, see #1375. You will have to bump version to 5. Also at the top of the XML file there is a sentence This document describes interface in version 4 which needs to be updated too.

I also wonder whether this shouldn't be more generic, e.g. looking at #1313, this will make OpenURI to handle more than schemes. Maybe something like having URISupported which is probably a terrible name, but just to give you the sense what I mean.

Also, OpenURI gives you an option to install an app that can possible handle the requested scheme if there is none on the system. With your SchemeSupported call, you will end up making Firefox to handle it itself, because you don't get to call the OpenURI. Just mentioning this as something to think about as I don't have a solution to this.

@grulja
Copy link
Contributor

grulja commented Jun 10, 2024

Also, maybe this can be permission based? To address @matthiasclasen concerns?

Like, first ask will popup a permission dialog, to ask whether you want to allow given application access to this information, store it for this app (app_id) and have all the following ones without any permission dialog needed in case the permission has been granted already?

That would mean using the generic Access portal for this.

@xhorak
Copy link
Contributor Author

xhorak commented Sep 24, 2024

@matthiasclasen we need to move forward with this. Please set some reviewer or check the PR on your own.

@swick
Copy link
Contributor

swick commented Oct 8, 2024

Seems perfectly reasonable. What are the concerns about exposing this by default (i.e. not gated behind some permission)?

The xml still has to be adjusted and a test would be nice, as pointed out by @grulja.

Copy link
Contributor

@grulja grulja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You also need to squash all the commits into one.

data/org.freedesktop.portal.OpenURI.xml Outdated Show resolved Hide resolved
@grulja
Copy link
Contributor

grulja commented Oct 9, 2024

I think the test is failing because you need to also bump the version in the test file. Still better would be to not just bump it, but also wrote a simple test.

@xhorak
Copy link
Contributor Author

xhorak commented Oct 9, 2024

Seems perfectly reasonable. What are the concerns about exposing this by default (i.e. not gated behind some permission)?

The xml still has to be adjusted and a test would be nice, as pointed out by @grulja.

Looking at the tests, they seems to be mostly implemented by libportal. Maybe we could add more when the libportal adds SchemeSupported implementation?

@grulja
Copy link
Contributor

grulja commented Oct 9, 2024

Seems perfectly reasonable. What are the concerns about exposing this by default (i.e. not gated behind some permission)?
The xml still has to be adjusted and a test would be nice, as pointed out by @grulja.

Looking at the tests, they seems to be mostly implemented by libportal. Maybe we could add more when the libportal adds SchemeSupported implementation?

You can make a simple sync DBus call there.

@xhorak xhorak force-pushed the systemhandler branch 3 times, most recently from fba4ed6 to e9010c7 Compare October 9, 2024 10:38
@xhorak xhorak requested a review from grulja October 10, 2024 06:29
Copy link
Contributor

@grulja grulja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I'm not an expert to GLib, but it looks sane to me.

NIT: still missing to bump the version in the xml file

data/org.freedesktop.portal.OpenURI.xml Outdated Show resolved Hide resolved
tests/openuri.c Outdated Show resolved Hide resolved
src/open-uri.c Outdated Show resolved Hide resolved
tests/openuri.c Outdated Show resolved Hide resolved
tests/openuri.c Outdated Show resolved Hide resolved
tests/openuri.c Outdated Show resolved Hide resolved
tests/openuri.c Outdated Show resolved Hide resolved
tests/openuri.c Show resolved Hide resolved
tests/openuri.c Outdated Show resolved Hide resolved
tests/openuri.c Outdated Show resolved Hide resolved
tests/openuri.c Outdated Show resolved Hide resolved
data/org.freedesktop.portal.OpenURI.xml Outdated Show resolved Hide resolved
@swick
Copy link
Contributor

swick commented Oct 11, 2024

In the file org.freedesktop.portal.OpenURI.xml at line 32 it still says version 4.

@swick
Copy link
Contributor

swick commented Oct 11, 2024

Otherwise this LGTM!

@xhorak
Copy link
Contributor Author

xhorak commented Oct 11, 2024

In the file org.freedesktop.portal.OpenURI.xml at line 32 it still says version 4.

Oh sorry, I've somehow only focused on the line 176.

@matthiasclasen matthiasclasen added this pull request to the merge queue Oct 11, 2024
Merged via the queue into flatpak:main with commit 1ae592a Oct 11, 2024
5 checks passed
@grulja
Copy link
Contributor

grulja commented Nov 1, 2024

@swick since 1.19.0 is not out yet, can we get this change in?

@jadahl
Copy link
Collaborator

jadahl commented Nov 1, 2024

can we get this change in?

This merge request was merged 3 weeks ago.

@smcv
Copy link
Collaborator

smcv commented Nov 1, 2024

since 1.19.0 is not out yet

1.19.0 is out. This change was merged after that, and will be in the 1.19.1 development release and the 1.20.0 stable release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Triaged
Development

Successfully merging this pull request may close these issues.

6 participants